Replace svg diagrams with mermaid where possible. #513
Replace svg diagrams with mermaid where possible. #513
Conversation
Thank you!Thank you for your pull request 😃 🤖 This automated message can help you check the rendered files in your submission for clarity. If you have any questions, please feel free to open an issue in {sandpaper}. If you have files that automatically render output (e.g. R Markdown), then you should check for the following:
Rendered Changes🔍 Inspect the changes: https://github.com/carpentries-incubator/python-intermediate-development/compare/md-outputs..md-outputs-PR-513 The following changes were observed in the rendered markdown documents: What does this mean?If you have source files that require output and figures to be generated (e.g. R Markdown), then it is important to make sure the generated figures and output are reproducible. This output provides a way for you to inspect the output in a diff-friendly manner so that it's easy to see the changes that occur due to new software versions or randomisation. ⏱️ Updated at 2026-02-12 16:37:35 +0000 |
There was a problem hiding this comment.
Thanks for following up on this so quickly @anenadic! The changes here look good in general to me. Some comments / questions:
- Do we also want to remove the corresponding
.svgversions of the Mermaid diagrams fromepisodes/fignow they are no longer used? - Are the HTML comments before each Mermaid fenced code block with the
alt="..."contents functional / do these addaltattributes to the outer HTML element of the Markdown that follows? If so is this documented somewhere - I couldn't see anything from a quick search in Workbench documentation but wasn't sure exactly what the right search terms to use were! - As it looks like the
accDescrsyntax in the Mermaid diagram sources already gets rendered to a<svg><desc>element, which appears to be the recommended way of adding descriptive text to SVG content, is there still value in having the same (or similar) text attached as analtattribute? From a don't-repeat-yourself perspective it feels like having the text duplicated will lead to it potentially getting out of sync, and from an accessibility perspective having multiple descriptions attached at different levels of a HTML element might actually be poorer from a user experience perspective for screen reader users? - For the section overview diagrams, in the preview of the updated Markdown rendered on GitHub, the reflowing of the longer bulleted text corresponding to the details of each section ends up a bit odd, making this difficult to read. In the SVGs previously used it looks like the corresponding nodes ended up automatically scaling width so this was less of an issue. At the moment we are using plain text labels for the nodes but Mermaid does support Markdown syntax in labels by enclosing within double-quotation marks and backticks e.g.
"`Markdown _formatted_ __text__ `". That might be worth employing here for the longer labels to make the bulleted text more readable.
Could do.
They were left there as the accDescr does not get translated into alt-text by Mermaid so I kept these. Also accDescr and alt-text are not the same thing (alt-text is more descriptive for use by screen readers and accDescr is just used as an image caption - but I tried to align the text between the two as much as possible). So not sure what we want to do with this.
|
|
I'll weigh in since I said I would have a look at Mermaid accessibility.
This isn't quite accurate. After a bit of a dig myself, there is a difference depending on how the SVG is included in the HTML. In our case, Mermaid inlines the SVG inside a I'm going to go ahead and make the requisite changes. I will also have a go at better formatting of text while I am there. |
|
A further update: I didn't appreciate that the Regardless, we should still get rid of the alt-text comments, and they should be put into |
|
Okay, I had a go at using the markdown formatting for nodes. Unfortunately, I failed to see how to left-align the text so that the bullet lists look better. @matt-graham can you confirm if at least the rendering in GitHub looks better? I think I am looking in the right place for that, but not quite sure (the rendered md outputs attached by the bot to this PR?) If we agree this looks fine, I can do the rest but ran out of time trying to figure out styling, which as always is a nightmare. |
There was a problem hiding this comment.
Thanks @bielsnohr for having a go with the Markdown list formatting and also investigating the alt / accDescr behaviour.
Annoyingly it seems that the Mermaid Markdown support is only for inline level formatting such as bold / italic not any block elements like lists, and further that in current Mermaid renderer versions setting the text alignment of labels using a style directive like style nodeid text-align:left has no effect. Looking at the SVG / HTML produced with a style directive added, while the style information does get correctly applied to the outer node element and inner span containing the label, it has no effect on the span element as this text-align only works for block display elements like divs, and the span element is nested inside a div for which text-align is fixed to center. From a previous StackOverflow question / answer on this issue it seems this wasn't always the case.
It seems like there is still some benefit to wrapping the labels in "`...`" as from the Mermaid documentation it seems like this should preserve any newlines in label text without needing to manually include <br> tags. Unfortunately it seems that common whitespace at the beginning of lines (for example from indenting diagram syntax) is not stripped and effect the rendered output. I have suggested changing the remaining section overview Mermaid diagrams to use Markdown labels for the bulleted lists and reduced the whitespace at the beginning of the list items to a single indent level, as this makes them display slightly more nicely on Mermaid Live and GitHub preview for me. We could also remove all indentation from the list items but then this makes the diagram syntax difficult to read.
Possibly given issues with formatting long text segments in Mermaid diagrams we might want to revisit whether to continue to have the detailed section breakdowns in the overviews in Mermaid rather than as a separate bulleted list, but I think this is probably better dealt with in another issue / PR.
| A(1. Setting up software environment) | ||
| A --> B(2. Verifying software correctness: | ||
| - Testing frameworks | ||
| - Automating & scaling testing: CI and GitHub Actions | ||
| - Debugging code) |
There was a problem hiding this comment.
| A(1. Setting up software environment) | |
| A --> B(2. Verifying software correctness: | |
| - Testing frameworks | |
| - Automating & scaling testing: CI and GitHub Actions | |
| - Debugging code) | |
| B("`2. Verifying software correctness: | |
| - Testing frameworks | |
| - Automating & scaling testing: CI and GitHub Actions | |
| - Debugging code`") | |
| A(1. Setting up software environment) --> B |
Matching using Markdown formatting as in 10-section-1-intro.md
| B --> C(3. Software development as a process | ||
| - Software requirements | ||
| - Software architecture & design | ||
| - Programming paradigms) |
There was a problem hiding this comment.
| B --> C(3. Software development as a process | |
| - Software requirements | |
| - Software architecture & design | |
| - Programming paradigms) | |
| C("`3. Software development as a process | |
| - Software requirements | |
| - Software architecture & design | |
| - Programming paradigms`") | |
| B --> C |
| C --> D(4. Collaborative development for reuse | ||
| - Code review | ||
| - Software documentation | ||
| - Software packaging & release | ||
| ) |
There was a problem hiding this comment.
| C --> D(4. Collaborative development for reuse | |
| - Code review | |
| - Software documentation | |
| - Software packaging & release | |
| ) | |
| D("`4. Collaborative development for reuse | |
| - Code review | |
| - Software documentation | |
| - Software packaging & release`") | |
| C --> D |
| D --> E(5. Managing software over its lifetime | ||
|
|
||
| - Issue reporting & prioritisation | ||
| - Agile development in sprints | ||
| - Software project management) |
There was a problem hiding this comment.
| D --> E(5. Managing software over its lifetime | |
| - Issue reporting & prioritisation | |
| - Agile development in sprints | |
| - Software project management) | |
| E("`5. Managing software over its lifetime | |
| - Issue reporting & prioritisation | |
| - Agile development in sprints | |
| - Software project management`") | |
| D --> E |
Fixes #507.